Repository navigation
fix: label Map attribute columns by their key instead of arrayElement(...) - #3163
Conversation
…(...) Selecting LogAttributes['service.name'] in the search results left the column named by ClickHouse's derived arrayElement(LogAttributes, 'service.name'), which is wider than the value under it and shows up wherever the result column name is read. renderSelectList now aliases each bare Map subscript in a string select after its key, so the column reads "service.name". The alias is emitted through quotedColumnName, which now escapes backslashes as well as double quotes, and is skipped when it would shadow a name ClickHouse could otherwise resolve (a table column, an alias CTE, or anything else named in the list), since ClickHouse resolves an identifier to an alias before a column. The same key selected from two maps is prefixed with the map. groupBy lists are untouched: GROUP BY x AS y is invalid, and the Kubernetes dashboard reads those rows by their derived names. Fixes ClickHouse#3095
🦋 Changeset detectedLatest commit: eb8cc31 The changes in this PR will be included in the next version bump. This PR includes changesets to release 3 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
@DevShoaib78 is attempting to deploy a commit to the HyperDX Team on Vercel. A member of the Team first needs to authorize it. |
github-actions
Bot
added
external
needs-vouch
|
Hi @DevShoaib78, thanks for the pull request! Before we review code from a first-time contributor we ask that a maintainer vouches for you, and you're not on our list yet. This PR stays open — it just isn't in the review queue until someone vouches. To get vouched, open an issue saying hello and what you're working on: https://github.com/hyperdxio/hyperdx/issues/new?template=introduce-yourself.md A maintainer will usually reply within a day or two, and then this PR gets picked up as normal. More detail in our contributing guide. |
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| ) | ||
| : await renderSelectList(chartConfig.select, chartConfig, metadata, { | ||
| mergeRatio: true, | ||
| aliasMapSubscripts: true, |
There was a problem hiding this comment.
🟠 major — Sorting a Map column now persists an alias-only ORDER BY that breaks as soon as the column leaves the SELECT
DBRowTable.tsx:610 gives a column that appears in the alias map the sort id "service.name", and DBSearchPage.tsx:1933 writes that verbatim into searchedConfig.orderBy (URL + saved search). Before this change the result name was arrayElement(LogAttributes, 'service.name') — a self-contained expression that stayed valid through any SELECT edit; now the ORDER BY only resolves while that exact alias is projected. Concretely: sort by the new service.name column, then remove it via the column menu (toggleColumn only rewrites select, and the form is reset from searchedConfig at DBSearchPage.tsx:1183, so orderBy survives) — every subsequent results query fails with "Unknown expression identifier 'service.name'", and saving the search stores the broken sort permanently. Fix: build the sort id from the expression (aliasMap[column] ?? column) instead of the quoted alias, or clear the sort when its alias is no longer in the select.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
There was a problem hiding this comment.
This is a real consequence of the change and I think it needs your call rather than a fix here. The sort id now carries the alias ("service.name") where it previously carried the whole arrayElement(...) expression, so a persisted sort breaks once that column leaves the SELECT.
The smallest fix I can see is in DBRowTable.tsx, which already computes aliasMap: resolve the sort id back through it when building orderBy.valueExpression, so what gets persisted is the expression rather than the alias. That is an app file this PR does not touch, so per AGENTS.md I have left it alone — happy to do it here if you would rather it landed together.
| const entries = splitAndTrimWithBracket(select).map(entry => { | ||
| const match = entry.match(BARE_MAP_SUBSCRIPT_RE); | ||
| const key = match == null ? undefined : unescapeSqlStringLiteral(match[2]); | ||
| return match == null || key == null || key.includes('"') |
There was a problem hiding this comment.
🔵 minor — A key containing a backslash is aliased, but nothing downstream can address the resulting column
The "-in-key skip exists because the alias has to round-trip through chSqlToAliasMap; \ has the same asymmetry. With the new escaping at line 180, LogAttributes['dir\\name'] projects AS "dir\\name" and ClickHouse names the column dir\name. Coming back, DBRowTable.tsx:610 re-quotes the meta name unescaped ("dir\name", which ClickHouse reads as dirname), and node-sql-parser's Postgresql dialect does not unescape backslashes inside a quoted identifier, so the alias-map key is dir\\name and never matches the meta name — sorting that column emits an ORDER BY that resolves to nothing. Add \\ to the skip alongside " (the escapes a backslash in the key test then pins the derived name instead).
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Fixed in 19292b5 — a key containing a backslash now keeps its derived name, same as one containing a double quote, and the doc comment explains both as the one round-trip problem. The existing backslash test was replaced with one pinning the unaliased form. quotedColumnName's backslash escaping is now unreachable from this path but still used for formula and operand column names, so I left it in place and added a test on the formula path to keep it covered.
| ...overrides, | ||
| }); | ||
|
|
||
| it('names the result columns after the selected keys', async () => { |
There was a problem hiding this comment.
🔵 minor — No test covers the alias-map round trip the whole feature rests on
The PR's claim that "no app change was needed" depends on chSqlToAliasMap (packages/common-utils/src/clickhouse/index.ts:939) recovering service.name -> ResourceAttributes['service.name'] from the rendered SQL — that map drives the sort id, useRowWhere's valueExpr, and the aliasWith clauses injected into the histogram/pattern/alert queries. The new tests only assert the emitted SQL text, and the author notes the browser side was never exercised, so nothing verifies that a dotted, double-quoted alias survives node-sql-parser. Feed the rendered ChSql from the names the result columns after the selected keys case into chSqlToAliasMap and assert the key/expression pair.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Added in 19292b5. The new test feeds the rendered ChSql from the "names the result columns after the selected keys" case straight into chSqlToAliasMap and asserts both pairs come back, so the dotted double-quoted alias surviving node-sql-parser is now pinned rather than assumed.
| // Undo the escapes a single-quoted ClickHouse literal carries for `'` and | ||
| // `\` — the two the app emits when it builds a subscript from a row's key | ||
| // (mergePath). Other C-style escapes stay as typed. | ||
| const unescapeSqlStringLiteral = (literal: string) => |
There was a problem hiding this comment.
🔵 minor — unescapeSqlStringLiteral is a third spelling of an unescape that already exists
escapeSqlString (packages/common-utils/src/core/utils.ts:82) is this function's exact inverse and lives in the shared module, and filters.ts:176 already hand-rolls the same \\ / '' unescape inline. Per the repo's DRY rule, export the new helper from core/utils.ts next to escapeSqlString and have filters.ts call it, rather than keeping a private third copy in renderChartConfig.ts.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Agreed that is the right long-term home, but it pulls core/utils.ts and filters.ts into a PR that touches neither. It is also not a pure move: filters.ts unescapes only \ and '', while this helper also unescapes \', so sharing one function changes filter parsing and would need its own tests. Happy to do it as a follow-up.
PR Review
3 finding(s): 🔴 0 critical · 🟠 0 major · 🔵 3 minor 2 posted as inline comment(s) on the changed lines. 1 listed below. Findings outside the changed lines1 minor
Severity is the reviewer's own estimate and is used for ordering, not filtering. No finding blocks merge automatically; a maintainer decides. |
Review round 1. Reserve the aliases a structured group-by binds before aliasing Map subscripts. renderGroupBy projects those aliases into the same SELECT, so a key equal to one of them produced two expressions under a single alias, which ClickHouse rejects. Skip a key containing a backslash as well as one containing a double quote, for the same reason: neither survives the round trip back through chSqlToAliasMap, so the recovered name never matches the column name ClickHouse reports and the alias cannot be addressed. Add a test feeding the rendered SQL through chSqlToAliasMap, which pins the round trip the feature rests on, and keep the backslash escaping in quotedColumnName covered by a formula-column test now that the Map path no longer reaches it.
Deep ReviewScope: The change is well-scoped, heavily unit-tested, and the guard rules (source-column / CTE / in-list identifier reservation, two-map prefixing, and skipping keys containing ✅ No critical issues found. 🟡 P2 — recommended
🔵 P3 nitpicks (2)
Reviewers (11): correctness, testing, maintainability, project-standards, security, api-contract, adversarial, kieran-typescript, previous-comments, agent-native, learnings-researcher. Testing gaps: No test exercises the renamed column key through a downstream consumer (API v2 / MCP / alert sample); coverage stops at the rendered SQL and the No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
| const out = entries.map(({ entry, subscript: s }) => { | ||
| if (s == null) return entry; | ||
| const alias = | ||
| (keyCount.get(s.key) ?? 0) > 1 ? `${s.column}.${s.key}` : s.key; |
There was a problem hiding this comment.
🔵 minor — A column's output name depends on its siblings, so adding an unrelated column silently renames an existing one and invalidates a persisted sort
keyCount makes the alias LogAttributes.http.method instead of http.method as soon as a second map is selected with the same key, and taken drops the alias entirely if any sibling entry mentions the key as an identifier. The search page persists the sort as text in the URL and in saved searches (orderBy: z.string() in SearchConfigSchema, DBSearchPage.tsx:189; the id is produced as "${column}" at DBRowTable.tsx:610), so a saved search sorted by "http.method" starts failing with "Unknown identifier" the moment the user adds SpanAttributes['http.method'] to the SELECT. Consider making the alias a function of the entry alone (always prefix with the map column, or disambiguate only the later duplicate) so an existing column's name never changes when a new one is added.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
That is deliberate and it is the safe direction: the reservation is a superset, so it can suppress an alias that would have been fine, but it cannot mint one that shadows a real name. I would rather a column keep its derived name than have a WHERE silently bind to the wrong expression. Happy to add a note at the call site saying so.
| // Undo the escapes a single-quoted ClickHouse literal carries for `'` and | ||
| // `\` — the two the app emits when it builds a subscript from a row's key | ||
| // (mergePath). Other C-style escapes stay as typed. | ||
| const unescapeSqlStringLiteral = (literal: string) => |
There was a problem hiding this comment.
🔵 minor — unescapeSqlStringLiteral is a third implementation of "undo escapeSqlString", and the existing one behaves differently
escapeSqlString lives in packages/common-utils/src/core/utils.ts:82, and its inverse is already inlined at packages/common-utils/src/filters.ts:176 (trimmed.slice(1, -1).replace(/\\\\/g, '\\').replace(/''/g, "'")). The new helper does the same operation but also handles \', which the filters.ts copy does not — so two call sites now disagree on how a key literal unescapes. Export one helper from core/utils next to escapeSqlString and have both filters.ts and renderChartConfig.ts use it.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| const entries = splitAndTrimWithBracket(select).map(entry => { | ||
| const match = entry.match(BARE_MAP_SUBSCRIPT_RE); | ||
| const key = match == null ? undefined : unescapeSqlStringLiteral(match[2]); | ||
| return match == null || key == null || /["\\]/.test(key) |
There was a problem hiding this comment.
🔵 minor — key == null can never be true
key is only undefined when match == null, which the same expression already tests first; match[2] is a non-optional capture group, so unescapeSqlStringLiteral always returns a string. Drop the key == null disjunct.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Correct, that is dead — match non-null guarantees group 2 exists. Holding off on the edit while I wait on the scope question I posted; I will fold it in with whatever is decided.
|
Round 2 done, and I am stopping here per AGENTS.md rule 4 to ask for a scope decision rather than keep chasing bot findings. Where it stands. Round 1's P1 was real and is fixed (group-by alias collision, confirmed by rendering the colliding SQL before fixing it), along with the backslash-key skip and the The thing I would like your call on. Two independent app-side breakages have now been found, and they share one root cause: app code takes the ClickHouse result-column name and feeds it back as a SQL expression, which worked only because that name used to be the self-contained
Both have the same shape of fix, resolving through the Three ways I can see to go, happy with any:
Also outstanding, all trivial and deliberately not fixed while this is open: a dead Worth flagging: I have no Docker here, so nothing has run against a live ClickHouse. The guard rules come from reading the code and the reviewers' reasoning, not from an executed query. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Thanks for taking a look at this @DevShoaib78 . I'm wondering if we can try to come at this from a slightly different angle, at least for the initial change: Instead of automatically applying these aliases in every query (via renderChartConfig, which has a very wide blast radius), we could instead focus on the search page select and add aliases there when adding columns to it. For example when the user clicks on a field to add it as a column from the side bar, the SELECT can be updated with the aliased expression. I don't think we need to automatically alias expressions that have been typed into the input without as alias - if the user is writing SQL, they can specify an alias, but if they're using a button the UI to select add a column to the table, it makes sense to provide a friendly name for it. 
That way the user can see in the SELECT input that the alias is being applied, the user can change the alias as desired if they want to, and there's no risk that this change breaks existing saved search / alert configs. The issue with denoise is interesting, I suspect there is an issue there even without these changes, I will take a look at that. |
Per review, aliasing every rendered query has too wide a blast radius (saved searches, alerts, dashboards all go through renderChartConfig). The alias moves to the search page instead, where it is only added when a column is picked from the UI and stays visible and editable in the SELECT.
…sidebars Per review: instead of rewriting every rendered query, add the alias on the search page at the moment a column is picked from the UI. Both "add column" actions (filters sidebar, row side panel) go through toggleColumn, which now writes a bare Map subscript as `Map['key'] AS "key"`. The alias is visible and editable in the SELECT, and hand-written SQL is untouched. The alias is skipped when it could change the query: a key named like a table column, a name already in the SELECT, or a key that would need escaping. The sidebar "is this column shown" checks now match a column with or without its alias, so the buttons keep reading "Remove" for an aliased column. displayedColumns itself is unchanged, since DBRowTable relies on it being the literal SELECT items.
|
Thanks @pulpdrew, that's a much better place for it. Reworked along those lines:
The PR is now +168/−9, mostly tests, and the description and preview steps are updated. I've left |
Deep Review✅ No critical issues found. The feature works on the happy path, the SQL-injection surface is closed (the 🟡 P2 -- recommended
🔵 P3 nitpicks (4)
Reviewers (10): correctness, adversarial, security, testing, maintainability, project-standards, kieran-typescript, previous-comments, agent-native, learnings-researcher. Six returned structured results (security, testing, maintainability, project-standards, agent-native, learnings); correctness, adversarial, kieran-typescript, and previous-comments were dispatched but had not returned at synthesis time — their domains (injection/escape round-trip, comma-in-alias splitting via Testing gaps:
No finding blocks merge automatically. A maintainer will expect P0/P1 findings in code this PR changes to be fixed; P2/P3 are your call -- fix or reply. Never fix findings about surrounding code here; reply instead. Do not widen the PR. How to respond |
| ? displayedColumns.filter(s => s !== selected) | ||
| : [ | ||
| ...displayedColumns, | ||
| withMapKeyAlias(column, displayedColumns, knownColumns), |
There was a problem hiding this comment.
🟠 major — Sorting an aliased Map column, then removing it, leaves a dangling ORDER BY "key" and breaks the query
Reachable with UI clicks only: add ResourceAttributes['service.name'] from the sidebar (SELECT becomes … AS "service.name"), click that column header to sort, then click the header's remove button. DBRowTable.tsx:610 gives an aliased column the id "service.name" (because aliasMap now has that key), onSortingChange (DBSearchPage.tsx:1943) persists orderBy: '"service.name" DESC' into the searched config/URL, and the removal branch here only rewrites select — so the next query is SELECT Timestamp, … FROM … ORDER BY "service.name" DESC with no such alias or column, and ClickHouse fails with Unknown identifier. Before this change the same column was sorted by its derived name arrayElement(ResourceAttributes, 'service.name'), a self-contained expression that survived removal. Fix in the removal branch: when the item being dropped carries an alias, clear the sort if searchedConfig.orderBy references that alias (fall back to defaultSearchConfig.orderBy) — or make DBRowTable's sort id the underlying expression (aliasMap[column]) instead of the quoted alias.
Nothing blocks merge automatically, but a maintainer will expect this fixed if it is a real defect in code this PR changes. If it is about surrounding code, reply and say so instead of patching. Do not widen the PR. How to respond
There was a problem hiding this comment.
Confirmed. The form's orderBy is bound to searchedConfig, so removing the column resubmitted "service.name" DESC. Fixed in cdc3047: removing a SELECT item whose alias the current sort is on resets the sort to the source default (orderByAfterRemovingSelectItem, with tests). It also covers a hand-typed alias removed from the header.
| const key = match[2].replace(/\\(.)/g, '$1'); | ||
| const selectedNames = selectItems.map(item => { | ||
| const alias = SELECT_ALIAS_REGEX.exec(item)?.[1]; | ||
| return alias ? alias.replace(/^["`]|["`]$/g, '') : item.trim(); |
There was a problem hiding this comment.
🔵 minor — Inline quote-stripping re-implements unquoteIdentifier
alias.replace(/^["]|["]$/g, '') is the operation unquoteIdentifier already performs (packages/common-utils/src/core/metadata.ts:120, exported from @hyperdx/common-utils/dist/core/metadata and named in the conventions' shared-code table). The inline version also differs: it strips unbalanced quotes (`x" → x), whereas unquoteIdentifier only unwraps a matched pair. Import and call unquoteIdentifier(alias) instead.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
Done in cdc3047: aliases now go through unquoteIdentifier via a small selectItemAlias helper.
| * table column (ClickHouse resolves an alias before a column), a name already | ||
| * in the SELECT list, or a key that would need escaping as an identifier. | ||
| */ | ||
| export const withMapKeyAlias = ( |
There was a problem hiding this comment.
🔵 minor — Header labelling is done by rewriting persisted SQL when a presentational rename already exists
DBRowTable already takes a columnNameMap keyed by the ClickHouse result column name and uses it for the header and CSV name (DBRowTable.tsx:513, :614; PatternSidePanel.tsx:55 uses it for exactly this purpose). Deriving that map in DBSqlRowTable from columnMap/aliasMap would relabel arrayElement(ResourceAttributes, 'service.name') → service.name without touching the query, which (a) removes the need for the three skip rules and the three alias-aware isColumnInSelect call sites, (b) keeps a format out of the URL and saved searches that stays valid forever once written, and (c) also fixes hand-typed SELECTs and a source's defaultTableSelectExpression, which this approach deliberately leaves showing arrayElement(...).
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
There was a problem hiding this comment.
A fair alternative, but @pulpdrew asked specifically for the alias to be written into the SELECT so it's visible and editable (#3163 (comment)), so I've kept that. Happy to switch to a presentational columnNameMap if the maintainers prefer it.
…for columns Review round on the sidebar alias: - Removing a SELECT item whose alias the current sort is on (a table header sort on an aliased column orders by `"alias"`) now resets the sort to the source default, instead of resubmitting an ORDER BY that no longer resolves. Also covers a hand-typed alias removed from the header. - No alias is added while the table's columns are unknown (still loading or failed to load), so a clash with a column name cannot slip through. - Aliases are unquoted with the shared unquoteIdentifier, and each skip rule is commented.
|
Review round addressed in cdc3047:
|
| ? displayedColumns.filter(s => s !== column) | ||
| : [...displayedColumns, column]; | ||
| // A column added from the UI can carry an alias, so match the expression too | ||
| const selected = displayedColumns.find( |
There was a problem hiding this comment.
🔵 minor — toggleColumn repeats the alias-aware match that isColumnInSelect already does
Add a findSelectItem(displayedColumns, column) helper to packages/app/src/utils.ts that returns the matching item, build isColumnInSelect on top of it, and call it here. As written, the "is it shown?" check in the sidebars (isColumnInSelect) and the "which item do I remove?" check here are two copies of s === column || selectItemExpression(s) === column. If one copy changes, a sidebar button can read "Remove" while clicking it adds a duplicate column.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| withMapKeyAlias(column, displayedColumns, knownColumns), | ||
| ]; | ||
| setValue('select', newSelectArray.join(', ')); | ||
| if (selected) { |
There was a problem hiding this comment.
🔵 minor — The new toggleColumn logic (removing an aliased item, resetting the sort) has no test
Add a DBSearchPage or DBRowJsonViewer test that adds a Map subscript, checks the SELECT gets AS "key", sorts by "key", then toggles the column again. It should assert the aliased item is removed and orderBy goes back to the default. The unit tests in utils.test.ts only cover the helpers, so they would still pass if toggleColumn used an exact === match or skipped orderByAfterRemovingSelectItem.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| ? displayedColumns.filter(s => s !== column) | ||
| : [...displayedColumns, column]; | ||
| // A column added from the UI can carry an alias, so match the expression too | ||
| const selected = displayedColumns.find( |
There was a problem hiding this comment.
🔵 minor — toggleColumn re-implements the isColumnInSelect match inline
The s === column || selectItemExpression(s) === column check is the same predicate as the new isColumnInSelect in utils.ts. Add a findColumnInSelect helper in utils.ts and have both toggleColumn and isColumnInSelect use it, so the button state and the add/remove action can't drift apart.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| withMapKeyAlias(column, displayedColumns, knownColumns), | ||
| ]; | ||
| setValue('select', newSelectArray.join(', ')); | ||
| if (selected) { |
There was a problem hiding this comment.
🔵 minor — New toggleColumn orderBy reset and aliasing have no test
The tests only cover the pure helpers. Nothing checks that removing an aliased column that is currently sorted resets the orderBy form value to defaultSearchConfig.orderBy, or that adding a column writes the aliased item into select. A wrong getValues/setValue wiring would still pass. Add a DBSearchPage/toggleColumn test for both paths.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
pulpdrew
left a comment
pulpdrew
left a comment
There was a problem hiding this comment.
LGTM - great change, thanks for the contribution
| ? displayedColumns.filter(s => s !== column) | ||
| : [...displayedColumns, column]; | ||
| // A column added from the UI can carry an alias, so match the expression too | ||
| const selected = displayedColumns.find( |
There was a problem hiding this comment.
🔵 minor — toggleColumn copies the matching rule that isColumnInSelect already implements
The s === column || selectItemExpression(s) === column rule now exists in two places: here and in isColumnInSelect (utils.ts). Move it into one exported finder, e.g. findSelectItem(displayedColumns, column), which returns the item. Build isColumnInSelect on top of it and use it here, so the side panel and sidebar "is shown" check can never disagree with what toggleColumn removes.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
| @@ -1737,13 +1740,38 @@ export function DBSearchPage() { | |||
|
|
|||
| const toggleColumn = useCallback( | |||
There was a problem hiding this comment.
🔵 minor — The new toggleColumn wiring has no test
The helpers are unit-tested, but nothing checks toggleColumn itself. No test removes an aliased item when it is given the bare expression, which is what the sidebar, side panel and header all send, or resets the form's orderBy to defaultSearchConfig.orderBy. Add a DBSearchPage test that toggles ResourceAttributes['service.name'] on, sorts by "service.name", toggles it off, and asserts the resulting select and orderBy.
Advisory. Fix if it is a small defect in code this PR changes; otherwise reply in the thread. Do not widen the PR or touch files it did not already change. How to respond
Summary
Fixes #3095. A Map attribute added as a column is labelled with ClickHouse's derived name,
arrayElement(ResourceAttributes, 'service.name'), in the results table.Following @pulpdrew's review, the alias is now added on the search page, at the moment a column is added from the UI, instead of automatically in
renderChartConfig. Both "add column" actions (the filters sidebar and the row side panel) go throughtoggleColumnonDBSearchPage, which now writes a bare Map subscript into the SELECT asResourceAttributes['service.name'] AS "service.name". The alias is visible and editable in the SELECT input, and SQL typed by hand is never rewritten, so saved searches, alerts and dashboards are untouched.The alias is left off when it could change the query: a key named like a column of the table (ClickHouse resolves an alias before a column in
WHERE/ORDER BY), a name already used in the SELECT (e.g. the same key from two maps), or a key containing"or\.Because an added column can now carry an alias, the three "is this column shown?" checks (row side panel, filters sidebar, nested filter groups) match the expression with or without its alias. Otherwise the button would read "Add" for a column that is already there. Removing a column from any of those places or from the table header works for both forms.
All the earlier
renderChartConfig/common-utilschanges are reverted.Screenshots or video
arrayElement(ResourceAttributes, 'service.name')service.nameHow to test on Vercel preview
Preview routes: /search
Steps:
service.nameattribute under ResourceAttributes and click its "Add ResourceAttributes['service.name'] column to results table" action.ResourceAttributes['service.name'] AS "service.name".service.name, notarrayElement(ResourceAttributes, 'service.name').References
arrayElement(LogAttributes, 'key')in the results table instead of the field name #3095Tests:
withMapKeyAliasandisColumnInSelectunit tests inpackages/app/src/__tests__/utils.test.ts(aliasing, backtick-quoted map column, escaped key, the three skip rules, non-subscripts left alone, aliased/unaliased matching). Not verified: no Docker on my machine, so I have not run the app against ClickHouse; the preview steps above are the end-to-end check.This was written with AI assistance; I reviewed the diff and ran the checks.